Improve Woo Tax - Tax Rate Table Population Logic #2906
Conversation
Remove dead code (allow_street_address_for_matched_rates). Remove WC 2.6 support. Apply WPCS fixes.
iyut
left a comment
iyut
left a comment
There was a problem hiding this comment.
The changes does a good job on better code organization and adding more strict type comparison. But I've left some comments that could improve this PR.
| == Changelog == | ||
|
|
||
| = 3.4.0 - 2025-xx-xx = | ||
| * Revamp - Tax Rate Table population logic. |
There was a problem hiding this comment.
Are we allowed to use Revamp? I'm still not sure about this.
There was a problem hiding this comment.
IDK but this is what actually fit the changes done here. * Tweak would mean small change. * Change would also fit.
There was a problem hiding this comment.
I see the code changes that it's intended to not save the zip code. What do you think if we put on the description of this PR and also put that on the changelog at least. I think the user should know about this. And potentially, we should ask this to A8C team too.
Yes, I'll reach out to change Woo Tax documentation and explain there that ZIP is not used in Tax Jurisdictions. Before that I want to have our internal validation that this change is good. |
| $json = wp_json_encode( $request_body ); | ||
| $cache_key = 'tj_tax_' . hash( 'md5', $json ); | ||
| $location = $this->get_normalized_location( $request_body ); | ||
| $location_catch_key = 'tj_jurisdictions_' . hash( 'md5', $location['to_country'] . $location['to_state'] . $location['to_zip'] . $location['to_city'] . $location['to_street'] ); |
There was a problem hiding this comment.
It seems the jurisdiction cache key generation differs between where it's set and where it's retrieved :
The GET (get_jurisdictions line 722):
$key = 'tj_jurisdictions_' . md5( strtoupper( implode( '', $location ) . $customer->get_shipping_address() ) );This line uses hash('md5', ...) while GET uses md5(), these produce the same result, but the inputs are completely different
I think using
$location['to_country'] . $location['to_state'] . $location['to_zip'] . $location['to_city'] . $location['to_street'] )Is safer that $customer->get_shipping_address() since this can be filtered
There was a problem hiding this comment.
Would be good to extract the cache key building to its own method to avoid mismatches going forward.
| if ( 400 == $response_code | ||
| $jurisdictions = isset( $response_body_decoded->tax->jurisdictions ) ? $response_body_decoded->tax->jurisdictions : null; | ||
| if ( ! empty( $jurisdictions ) ) { | ||
| set_transient( $location_catch_key, $jurisdictions, $this->cache_time ); |
There was a problem hiding this comment.
| set_transient( $location_catch_key, $jurisdictions, $this->cache_time ); | |
| set_transient( $location_cache_key, $jurisdictions, $this->cache_time ); |
| } else { | ||
| $this->_error( 'Error retrieving the tax rates. Received (' . $response['response']['code'] . '): ' . $response['body'] ); | ||
| $this->_error( 'Error retrieving the tax rates. Received (' . $response_code . '): ' . $response['body'] ); | ||
| } |
There was a problem hiding this comment.
Missing return statement
Description
Revamped the Tax Rate Table population logic, improving handling of automated taxes, backend order calculations, and tax location normalization.
Changes:
wp_unslashand consistent uppercasing.json_encodewithwp_json_encode, unified style and comment formatting, and removed obsolete conditionals.Related issue(s)
Closes https://linear.app/a8c/issue/WOOSHIP-1819/improve-woo-tax-tax-rate-table-population-logic
Steps to reproduce & screenshots/GIFs
Test tax calculation for multiple scenarios:
Checklist
changelog.txtentry addedreadme.txtentry added